fix(ImageBuf): nmiplevels() returned 0 for non-IC ImageBuf - #5437

I'm worried that this is quite expensive, since seek_subimage will read/decode all the MIP image metadata. It will make opening a file with an ImageBuf bear the expense of seek_subimage calls even if no mip levels will ever be needed.

I'm wondering a few things:

• Do we have a definitive list of the file formats we support that can have MIP levels at all, and for those that do, whether the number can be known up front or must be iterated over all levels to find out?
• I think certainly we don't want to do the iteration unless the format supports MIP-maps, the image IS a mip map, AND we can't tell the number of mip levels up front.
• I wonder if seek_subimage is a lot less efficient than calling spec_dimensions, for two reasons: (a) spec_dimensions doesn't reset the "current" MIP level, and can be thread-concurrent (no locking needed), and generally doesn't need to re-seek back to the original level again when it's done; (b) because spec_dimensions only returns a partial spec giving the "size", it either is (hopefully) or could be made (future) less expensive by knowing it can skip work related to retrieval of the full metadata for that MIP level.

Or, another possibility is that we don't do it as part of the read() at all, but instead use the separate ImageBuf::nmiplevels() method for this. Right now it returns m_nmiplevels, which obviously might not know. But it could instead figure it out by doing the seeks or spec_dimensions or whatever, if it doesn't already set it.

In other words, we split the responsibilities to figure this out lazily:

• The ImageBuf, upon init_spec(), knows the number of MIP levels, or that there are MIP levels but it doesn't know the exact number yet (some file formats have an inexpensive way to know for sure, others don't, and that's why it could be in either state).
• ImageBuf::nmiplevels() returns the cached number if it was known for sure from init_spec(), or if it has already been computed. Otherwise, it does the potentially expensive thing of seeking or spec_dimensions'ing, saving the result and returning it.

So that would prevent/postpone the expensive kind of search until the occasion that the user actually needs to know the answer, conveyed by calling nmiplevels().

Thoughts?

Oops, it was an oversight. Here are my thoughts after some investigation:

• Currently mipmap supporting formats are: DDS, OpenEXR, Ptex, and TIFF.

  For DDS, OpenEXR, and Ptex, the number of mip levels can be known up front.

  • DDS: it is held privately in the ImageInput subclass and thrown away currently.
  • OpenEXR: commented out spec attribute setup for "oiio:miplevels" exists (comment says unit tests fail).

    OpenImageIO/src/openexr.imageio/exrinput.cpp

    Lines 1148 to 1151 in 1992123

    	//! Add the number of miplevels as an attribute for the first miplevel.
    	//! TOFIX: adding the following attribute breaks unit tests
    	// if (m_miplevel == 0 && part.nmiplevels > 1)
    	// m_spec.attribute("oiio:miplevels", part.nmiplevels);

  • Ptex: already sets the spec attribute for "oiio:miplevels".
    

    OpenImageIO/src/ptex.imageio/ptexinput.cpp

    Lines 445 to 447 in 1992123

    	// Add the number of miplevels as an attribute for the first miplevel.
    	if (miplevel == 0 && nmiplevels > 1)
    	m_spec.attribute("oiio:miplevels", nmiplevels);

  For TIFF, an iteration would be needed.

• I didn't realize I could use spec_dimensions() like that, but after looking at it, I think it will work great for the iteration case.

• Having a lazy nmiplevels() makes sense for iteration-required formats. And for others, grabbing the values during init_spec() would be cheap.

So I think the next steps are:

1. Re-enable exr's spec attribute setup + add new DDS spec attribute setup (oiio:miplevels).
2. In init_spec(), replace the seek_subimage() iteration with the following: (1) check supports("mipmap"), set the count to 1 if not, (2) check spec.get_int_attribute("oiio:miplevels"), save if it exists.
3. In IB::nmiplevels(), (1) validate_spec() (which calls init_spec() if m_spec_valid is false) (2) check if the value is already set (3) otherwise compute it by iteration with spec_dimensions().

Am I missing something?.....

Or would checking spec.get_int_attribute in IB::nmiplevels() instead be better?